Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: improve test-fs-open-flags #10908

Closed
wants to merge 1 commit into from
Closed

test: improve test-fs-open-flags #10908

wants to merge 1 commit into from

Conversation

vinimdocarmo
Copy link
Contributor

  • use arrow funcion instead of function expression
  • add second argumento to method assert.throws
  • check error messages from beginning to the end using ^ and $
Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@vinimdocarmo
test: improve test-fs-open-flags
cd81e86 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 20, 2017
edsadr
edsadr requested changes Jan 20, 2017
View reviewed changes
Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes but looks good =)

test/parallel/test-fs-open-flags.js
);

assert.throws(
() => stringToFlags(true),
/Unknown file open flag: true/
/^Error: Unknown file open flag: true$/
);

assert.throws(
() => stringToFlags(null),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jan 20, 2017
@vinimdocarmo
Copy link
Contributor Author

vinimdocarmo commented Jan 20, 2017 •
edited
Loading

@edsadr, I only added new lines between lines 40 and 43. Should I update all the lines pointed out by you?

@Trott
Copy link
Member

Trott commented Jan 20, 2017

I only added new lines between lines 40 and 43. Should I update all the lines pointed out by you?

I think my preference would be that you leave it as you currently have it, keeping the style of the existing file with the first argument on a line by itself. If you move all the args to the same line as the function call's opening parenthesis, then the linter will make you align all subsequent arguments, resulting in more churn. An then there will be yet more churn if that additional indentation causes a line to extend beyond 80 characters. So I think I actually would rather you keep it just like you already have it.

That said, I don't feel strongly about it, so if @edsadr or anyone else has compelling reasons for wanting those changes, I'm OK with it.

Trott
Trott approved these changes Jan 20, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

@Trott
Copy link
Member

Trott commented Jan 20, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/5962/

@vinimdocarmo
Copy link
Contributor Author

@Trott, can you give me a feedback about the CI?

edsadr
edsadr approved these changes Jan 20, 2017
View reviewed changes
Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinimdocarmo the CI is green, I was concerned about identation but as @Trott explains this could lead to more linting problems, then is fine and then.. LGTM 👍🏽

@vinimdocarmo
Copy link
Contributor Author

vinimdocarmo commented Jan 20, 2017 •
edited
Loading

Great! Thanks, @Trott @edsadr. Just waiting to see whether this PR lands or not.

@Trott
Copy link
Member

Trott commented Jan 20, 2017

@vinimdocarmo PRs stay open for at least 48 hours and that goes up to 72 hours if it stretches into a weekend like this one will. (Fell free to leave a "bump" comment if it's still open on Monday.)

@vinimdocarmo
Copy link
Contributor Author

Okay, @Trott.

edsadr pushed a commit that referenced this pull request Jan 22, 2017
@vinimdocarmo @edsadr
test: improve test-fs-open-flags
a024104 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: #10908
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@edsadr
Copy link
Member

edsadr commented Jan 22, 2017 •
edited
Loading

Landed in a024104, thanks for your contribution @vinimdocarmo

@edsadr edsadr closed this Jan 22, 2017
@vinimdocarmo vinimdocarmo deleted the test-updates branch January 22, 2017 21:51
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@vinimdocarmo @italoacasas
test: improve test-fs-open-flags
104c771 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: nodejs#10908
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@vinimdocarmo @italoacasas
test: improve test-fs-open-flags
ffd585e 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: nodejs#10908
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@vinimdocarmo @italoacasas
test: improve test-fs-open-flags
aa4b028 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: nodejs#10908
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@vinimdocarmo @italoacasas
test: improve test-fs-open-flags
c22e051 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: nodejs#10908
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Jun 19, 2017
Closed
2 tasks
gibfahn pushed a commit that referenced this pull request Jun 19, 2017
@vinimdocarmo @gibfahn
test: improve test-fs-open-flags
a5b8d1b 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: #10908
Backport-PR-URL: #13785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@vinimdocarmo @gibfahn
test: improve test-fs-open-flags
a26eb34 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: #10908
Backport-PR-URL: #13785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@vinimdocarmo @MylesBorins
test: improve test-fs-open-flags
6020e72 
* use arrow funcion instead of function expression
* add second argument to method assert.throws
* check error messages from beginning to the end using ^ and $

PR-URL: #10908
Backport-PR-URL: #13785
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@edsadr edsadr edsadr approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@vinimdocarmo @Trott @edsadr @lpinca @targos @cjihrig @mscdex @MylesBorins @gibfahn @nodejs-github-bot